[std] add minimal CLI parser std.cli#24881
Conversation
|
I recommend giving it a trial run by porting all the files in tools/ to use this. You can test at least that those compile successfully with |
|
Currently, just josh@nixos:~/dev/zig$ zig run --zig-lib-dir lib/ tools/docgen.zig -- --help
Usage: docgen [options] input output
Generates an HTML document from a docgen template.
Options:
--code-dir dir Path to directory containing code example outputs
--help Print this help and exit
error: Help
/home/josh/dev/zig/lib/std/cli.zig:275:13: 0x118c7af in innerParse__anon_25090 (std.zig)
return error.Help;
^
/home/josh/dev/zig/lib/std/cli.zig:159:5: 0x118399b in parseIter__anon_24026 (std.zig)
return innerParse(Args, arena, iter, prog, options.writer);
^
/home/josh/dev/zig/lib/std/cli.zig:115:5: 0x1169426 in parse__anon_22563 (std.zig)
return parseIter(Args, arena, &iter, options);
^
/home/josh/dev/zig/tools/docgen.zig:41:18: 0x1166557 in main (docgen.zig)
const args = try std.cli.parse(Args, arena, .{});
^This appears to be done unconditionally for all error codes: Lines 638 to 640 in 623290e Is there any appetite for special casing the cli errors If we don't want to modify |
sounds like #24510 |
Just my 2c, but if I propagated
I think that would be the simplest, most local solution. |
EDIT: |
I'm no longer calling |
89616c5 to
6449e40
Compare
| /// First positional (non-named) argument: | ||
| input: [:0]const u8 = "", | ||
| /// Second positional argument is declared as optional: | ||
| reptitions: u32 = 1, |
There was a problem hiding this comment.
Potential typo of "repetitions"? (completely inconsequential)
There was a problem hiding this comment.
🤦 😂 fixed again (in the next commit). thanks yall.
60d5a4a to
6be202e
Compare
thejoshwolfe
left a comment
There was a problem hiding this comment.
per @andrewrk 's suggestion, i ported almost all the tools/ to this new cli parsing interface to try it out, and my general thoughts are:
- it's easy to support
--helpby callingstd.cli.parse()even with an emptyArgsstruct, which means users get a more consistent/predictable interface. previously some tools were ignoring--helpor interpreting it as a file path or something (and then hopefully crashing). - the generated
--helpoutput tends to be more verbose, such as listing all the arguments twice and stating that they're required. not always an improvement. - the use case of a fixed number of positional args that have specific names is a very good fit in this
tools/directory.
one tool was not touched yet: tools/doctest.zig. This is due to that tool using single-letter -i and -o named arguments, which means this PR would introduce an API break to that tool, and require updating all callers including build.zig at least. i will attempt this in a follow up commit, but didn't want to test changing too many things at once.
There was a problem hiding this comment.
before:
$ zig run --zig-lib-dir lib/ tools/docgen.zig -- --help
Usage: docgen [options] input output
Generates an HTML document from a docgen template.
Options:
--code-dir dir Path to directory containing code example outputs
-h, --help Print this help and exitafter:
$ zig run --zig-lib-dir lib/ tools/docgen.zig -- --help
usage: docgen --code-dir=string input output
Generates an HTML document from a docgen template.
positional arguments:
input string. required
output string. required
named arguments:
--code-dir=string required. Path to directory containing code example outputs
--help print this help and exit- fixed
--code-dirincorrectly appearing optional. - lost the
dirmetavar for--code-dir, now calledstring.
There was a problem hiding this comment.
before:
$ zig run --zig-lib-dir lib/ tools/dump-cov.zig -- --help
thread 296647 panic: index out of bounds: index 2, len 2
/home/josh/dev/zig.master/tools/dump-cov.zig:21:31: 0x1148d3d in main (dump-cov.zig)
const cov_file_name = args[2];
^
/home/josh/dev/zig.master/lib/std/start.zig:627:37: 0x114bbf9 in posixCallMainAndExit (std.zig)
const result = root.main() catch |err| {
^
/home/josh/dev/zig.master/lib/std/start.zig:232:5: 0x1146be1 in _start (std.zig)
asm volatile (switch (native_arch) {
^
???:?:?: 0x0 in ??? (???)
Aborted (core dumped)after:
$ zig run --zig-lib-dir lib/ tools/dump-cov.zig -- --help
usage: dump-cov exe_file cov_file
positional arguments:
exe_file string. required
cov_file string. required
named arguments:
--help print this help and exitThere was a problem hiding this comment.
before:
$ zig run --zig-lib-dir lib/ tools/fetch_them_macos_headers.zig -- --help
info: fetch_them_macos_headers [options] [cc args]
Options:
--sysroot Path to macOS SDK
General Options:
-h, --help Print this help and exitafter:
$ zig run --zig-lib-dir lib/ tools/fetch_them_macos_headers.zig -- --help
usage: fetch_them_macos_headers [options] [cc_args...]
positional arguments:
cc_args string. can be specified multiple times
named arguments:
--sysroot=string default: ''. Path to macOS SDK
--help print this help and exit- no longer support
-h - slightly more clear that
cc argsnow calledcc_argscan be specified multiple times.
There was a problem hiding this comment.
before:
$ zig run --zig-lib-dir lib/ tools/gen_macos_headers_c.zig -- --help
info: gen_macos_headers_c [dir]
General Options:
-h, --help Print this help and exitafter:
$ zig run --zig-lib-dir lib/ tools/gen_macos_headers_c.zig -- --help
usage: gen_macos_headers_c dir
positional arguments:
dir string. required
named arguments:
--help print this help and exit- fixed
[dir]appearing optional - more verbose for little gain
There was a problem hiding this comment.
before:
josh@nixos:~/dev/zig.master$ r gen_outline_atomics.zig
$ zig run --zig-lib-dir lib/ tools/gen_outline_atomics.zig -- --help
//! This file is generated by tools/gen_outline_atomics.zig.
const builtin = @import("builtin");
const std = @import("std");
const common = @import("common.zig");
const always_has_lse = builtin.cpu.has(.aarch64, .lse);
[... ~2000 lines of output]after:
$ zig run --zig-lib-dir lib/ tools/gen_outline_atomics.zig -- --help
usage: gen_outline_atomics
named arguments:
--help print this help and exitThere was a problem hiding this comment.
before:
Usage: /home/josh/.cache/zig/o/80422d926c8fa91c39a5e0ca7c41471c/update-linux-headers [--search-path <dir>] --out <dir> --abi <name>
--search-path can be used any number of times.
subdirectories of search paths look like, e.g. x86_64-linux-gnu
--out is a dir that will be created, and populated with the resultsafter:
$ zig run --zig-lib-dir lib/ tools/update-linux-headers.zig -- --help
usage: update-linux-headers [options] --out=string
named arguments:
--search-path=string can be specified multiple times. subdirectories of search paths look like, e.g. x86_64-linux-gnu
--out=string required. a dir that will be created, and populated with the results
--help print this help and exit- fixed incorrect
--abiparameter, probably a copy-paste error fromprocess_headers.zig.
There was a problem hiding this comment.
before:
$ zig run --zig-lib-dir lib/ tools/update_clang_options.zig -- --help
Usage: /home/josh/.cache/zig/o/2988e875c2d7781b249c2e4a40efde00/update_clang_options /path/to/llvm-tblgen /path/to/git/llvm/llvm-project
Alternative Usage: zig run /path/to/git/zig/tools/update_clang_options.zig -- /path/to/llvm-tblgen /path/to/git/llvm/llvm-project
Prints to stdout Zig code which you can use to replace the file src/clang_options_data.zig.after:
$ zig run --zig-lib-dir lib/ tools/update_clang_options.zig -- --help
usage: update_clang_options /path/to/llvm-tblgen /path/to/git/llvm/llvm-project
Prints to stdout Zig code which you can use to replace the file src/clang_options_data.zig.
positional arguments:
/path/to/llvm-tblgen string. required
/path/to/git/llvm/llvm-project string. required
named arguments:
--help print this help and exit- more verbose
- removed
zig runtutorial.
There was a problem hiding this comment.
before:
$ zig run --zig-lib-dir lib/ tools/update_cpu_features.zig -- --help
Usage: /home/josh/.cache/zig/o/8ecc2bee7f3cbf32ac512f1adaa7aa7e/update_cpu_features /path/to/llvm-tblgen /path/git/llvm-project /path/git/zig [zig_name filter]
Updates lib/std/target/<target>.zig from llvm/lib/Target/<Target>/<Target>.td .
On a less beefy system, or when debugging, compile with -fsingle-threaded.after:
$ zig run --zig-lib-dir lib/ tools/update_cpu_features.zig -- --help
usage: update_cpu_features /path/to/llvm-tblgen /path/git/llvm-project /path/git/zig [zig_name_filter]
Updates lib/std/target/<target>.zig from llvm/lib/Target/<Target>/<Target>.td .
On a less beefy system, or when debugging, compile with -fsingle-threaded.
positional arguments:
/path/to/llvm-tblgen string. required
/path/git/llvm-project string. required
/path/git/zig string. required
zig_name_filter string. default: ''
named arguments:
--help print this help and exit- more verbose
There was a problem hiding this comment.
before:
$ zig run --zig-lib-dir lib/ tools/update_crc_catalog.zig -- --help
Usage: /home/josh/.cache/zig/o/29fd83f330fee72e83a7806ac7d8e0c2/update_crc_catalog /path/git/zigafter:
$ zig run --zig-lib-dir lib/ tools/update_crc_catalog.zig -- --help
usage: update_crc_catalog /path/git/zig
positional arguments:
/path/git/zig string. required
named arguments:
--help print this help and exit- less verbose arg0
- more verbose everything else
There was a problem hiding this comment.
before:
$ zig run --zig-lib-dir lib/ tools/update_freebsd_libc.zig -- --help
thread 298062 panic: index out of bounds: index 2, len 2
/home/josh/dev/zig.master/tools/update_freebsd_libc.zig:21:30: 0x1135994 in main (update_freebsd_libc.zig)
const zig_src_path = args[2];
^
/home/josh/dev/zig.master/lib/std/start.zig:627:37: 0x11377c9 in posixCallMainAndExit (std.zig)
const result = root.main() catch |err| {
^
/home/josh/dev/zig.master/lib/std/start.zig:232:5: 0x1131be1 in _start (std.zig)
asm volatile (switch (native_arch) {
^
???:?:?: 0x0 in ??? (???)
Aborted (core dumped)after:
$ zig run --zig-lib-dir lib/ tools/update_freebsd_libc.zig -- --help
usage: update_freebsd_libc freebsd_src_path zig_src_path
positional arguments:
freebsd_src_path string. required
zig_src_path string. required
named arguments:
--help print this help and exitapproximately the same for the remaining three tools:
update_glibc.zig
update_mingw.zig
update_netbsd_libc.zig
| ; | ||
| const Args = struct { | ||
| named: struct { | ||
| sysroot: []const u8 = "", |
There was a problem hiding this comment.
At a glance it might be nicer to instead type it ?[]const u8 and default to null.
Then the code wouldn't have to check for length 0, and could use orelse instead.
iiuc, passing --sysroot="" leads to auto-detection, with that change I assume it would lead to an error and have to be omitted.
Is there a code or interface reason that I'm missing that makes "" more intuitive for auto-detection?
There was a problem hiding this comment.
the reason is that i ported code that was expecting null after argument parsing, and so i preserved those semantics. optional arguments declared as ?[]const u8 = null makes a lot of intuitive sense, but i explained why i believe it is a misfeature in #24601.
it could be that the pattern here will be frequent where users want to conflate "" with null, like i'm doing, but a layer of code to translate from a deserialization system into the type you want to use in your code base is a common thing that doesn't mean the deserialization system is missing a feature.
if you disagree that lack of support for null is a misfeature, let's discuss it #24601. the real actual use cases you're pointing to in this review are evidence against my stance.
There was a problem hiding this comment.
iiuc, passing
--sysroot=""leads to auto-detection
wait, are you saying that "" is different that not being specified? if so, then i've broken something. i'll take a look.
There was a problem hiding this comment.
well, i can't tell whether cc -isysroot '' is going to do something meaningful, so i don't know if i broke it or not. on my system, i get this:
error: no SDK found; you can provide one explicitly with '--sysroot' flag
based on the name and lack of comments (git log didn't help either), i guess this is supposed to run on MacOS perhaps? pinging @alexrp to shed insight. is --sysroot="" supposed to be different from omitting the --sysroot argument entirely?
There was a problem hiding this comment.
iiuc, passing
--sysroot=""leads to auto-detectionwait, are you saying that
""is different that not being specified?
No, my bad, should have been more specific - I was just talking about the behavior of the new code.
I'm not familiar with the old behavior either, but think it's unlikely that "" had any special meaning/behavior previously.
| @"zig-lib-dir": []const u8 = "", | ||
| @"debug-zcu": bool = false, | ||
| @"debug-dwarf": bool = false, | ||
| @"debug-link": bool = false, | ||
| preserve_tmp: bool = false, | ||
| @"zig-cc-binary": []const u8 = "", |
There was a problem hiding this comment.
same here, @"zig-lib-dir" and @"zig-cc-binary" could be ?[]const u8 and start out as null, then the code could use orelse instead of checking for length 0.
| @"/path/to/llvm-tblgen": [:0]const u8, | ||
| @"/path/git/llvm-project": [:0]const u8, | ||
| @"/path/git/zig": [:0]const u8, | ||
| zig_name_filter: []const u8 = "", |
There was a problem hiding this comment.
same here, could be : ?[]const u8 = null
|
I may be missing something, but why are we dealing with the questionable |
I think it's meant to be a more flexible API. Some programs might want to use |
|
This branch is not passing standard library test suite and hasn't been touched in over 30 days. Do you need help figuring out why the checks are failing? |
|
this PR is in design discussion hell in #24601 . I have a plan to proceed despite the deadlock, but i haven't prioritized it recently. if the open PR is causing friction in the project management, I can open a new (rebased) PR at a later time. |
Closes #24601